[ECP-9177] Implement webhook clean-up cronjob for old webhooks #2837
Conversation
candemiralp
had a problem deploying
to
internal
GitHub Actions
Failure
candemiralp
had a problem deploying
to
internal
GitHub Actions
Failure
|
# Conflicts: # Helper/Config.php # etc/di.xml
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
| return $items->getItems(); | ||
| } catch (LocalizedException $e) { | ||
| $errorMessage = sprintf( | ||
| __('An error occurred while providing notifications older than %s days!'), |
There was a problem hiding this comment.
Can we add the store name or ID here? Maybe that's useful to see in the log line to be able to debug
There was a problem hiding this comment.
Hello @acampos1916, thank you for the review.
Notification entity is independent from the stores and we don't have a store value here. Similarly, this catch block targets the issues while fetching the entities. Most probably, there won't be any notification entity for any descriptive features to add to the logs.
|
| $isSuccessfullyDeleted = $this->adyenNotificationRepository->delete($notificationToCleanup); | ||
|
|
||
| if ($isSuccessfullyDeleted) { | ||
| $message = __('%1: Notification with entityId %2 has been deleted.', |
There was a problem hiding this comment.
Can we add also here one extra info: Notification with entityId %2 has been deleted because it was processed/received X days ago.
Probably means getting that field from the provider
| $this->notificationsProvider = null; | ||
| } | ||
|
|
||
| public function testProvideSuccess() |
There was a problem hiding this comment.
Is this method missing an assertion?
| <config_path>payment/adyen_abstract/cleanup_old_webhooks</config_path> | ||
| <tooltip> | ||
| Webhooks older than certain days will be removed from the database by a cronjob if this feature is enabled. | ||
| The default value is 90 days and this can be configured by overriding `payment/adyen_abstract/required_days_old_webhooks` configuration path. |
There was a problem hiding this comment.
Are we then hiding required_days_old_webhooks ?
|
Two extra details we can add to this PR: A confirm dialog when the config gets saved so that the admin user acknoledges that webhooks older than X will be deleted. A message on the Webhook list view to inform that webhooks newer than X days are being listed and that older than X are being removed by this configuration (link to the config) |
Hey @acampos1916, I like this idea. It will foster the merchant experience a lot. Let me have look into that one. Cheers. |
Description
The plugin stores asynchronous payment events (webhooks) in
adyen_notificationDB table. However, this table might end-up with huge amount of data and requires clean-up time to time.This PR introduces a cronjob to clean-up old notifications older than 90 days from the database if it's enabled. Please note that this feature is disabled by default and can be enabled from the plugin configuration.
Tested scenarios